[Accessibility] Allow setting of the aria-label of table row selection checkboxes#2043
Conversation
PhilippBaranovskiy
requested review from
cchaos,
chandlerprall and
peteharverson
peteharverson
left a comment
peteharverson
left a comment
There was a problem hiding this comment.
LGTM. This will allow elastic/kibana#28390 to be addressed for example.
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
The changes LGTM. I wonder if we should add a quick sentence about the importance of adding the ariaLabel prop in the docs here:
Agree, I will drop a line there. |
chandlerprall
left a comment
chandlerprall
left a comment
There was a problem hiding this comment.
Pulling the value from item.ariaLabel requires the dataset to be modified for the table, and this is something we have seen engineers push back on. Instead, we prefer having lookup functions that are passed the item and return the desired value. As an example, look at the table's use of selection.selectableMessage.
Which leads to another thought, it appears we're already using selection.selectableMessage for this exact case, but it isn't being added to aria-label. Should we re-use that value? Related: #2021
|
@chandlerprall, such a brilliant point, thanks for that! |
|
@cchaos what are you thoughts on re-using the existing |
|
jenkins, test this |
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
what are you thoughts on re-using the existing title value on the selection checkbox for aria-label?
I am fine with that. They are typically the same. I can't see a reason why they should ever be different.
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
|
@rockfield Let's remove the new method re-use the existing |
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
|
@chandlerprall @cchaos Please, could you have a look and confirm? |
chandlerprall
left a comment
chandlerprall
left a comment
There was a problem hiding this comment.
One last change, sorry!
chandlerprall
left a comment
chandlerprall
left a comment
There was a problem hiding this comment.
Changes LGTM, thanks @rockfield !
4 participants
Fixes #1697
Summary
To allow setting aria-label attribute override default
Select this rowvalue I've put a conditional assigning value.Now if selection.selectionMessage is provided, then
aria-labelvalue would be equal to that function's output.Otherwise, the default value is applied.
Checklist
This was checked in mobileThis was checked in IE11This was checked in dark modeAny props added have proper autodocsDocumentation examples were addedThis was checked for breaking changes and labeled appropriatelyThis required updates to Framer X components